Refactor cmake-rn to use a platform abstraction
#189
Conversation
kraenhansen
added
Apple 🍎
🦋 Changeset detectedLatest commit: 2ca36b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
kraenhansen
changed the title
cmake-rn platform abstractioncmake-rn to use a platform abstraction
kraenhansen
force-pushed
the
kh/cmake-rn-platforms
branch
from
2459faf to
0f7be36
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the cmake-rn package to use a platform abstraction layer, introducing interfaces for supporting different build targets in a more structured way. This prepares the codebase for future platform additions and improves separation of concerns.
- Introduces a
Platforminterface with platform-specific implementations for Android and Apple targets - Refactors CLI to use the new platform abstraction instead of direct triplet handling
- Consolidates weak-node-api path handling through a centralized module
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/host/src/node/weak-node-api.ts | New module providing centralized weak-node-api path resolution |
| packages/host/src/node/prebuilds/triplets.ts | Adds type guard function and improves existing triplet type guards |
| packages/host/src/node/path-utils.ts | Improves error message and adds input validation |
| packages/host/src/node/index.ts | Updates exports to include new utilities |
| packages/host/package.json | Updates weak-node-api export path |
| packages/ferric/src/cargo.ts | Refactors to use centralized weak-node-api path |
| packages/cmake-rn/tsconfig*.json | Restructures TypeScript configuration with separate configs |
| packages/cmake-rn/src/weak-node-api.ts | Updates to use centralized weak-node-api path |
| packages/cmake-rn/src/platforms/types.ts | Defines platform abstraction interface |
| packages/cmake-rn/src/platforms/apple.ts | Apple platform implementation |
| packages/cmake-rn/src/platforms/android.ts | Android platform implementation |
| packages/cmake-rn/src/platforms.ts | Platform registry and utilities |
| packages/cmake-rn/src/platforms.test.ts | Tests for platform utilities |
| packages/cmake-rn/src/cli.ts | Major refactor to use platform abstraction |
| packages/cmake-rn/src/apple.ts | Removed (functionality moved to platforms/apple.ts) |
| packages/cmake-rn/src/android.ts | Removed (functionality moved to platforms/android.ts) |
| import type { program } from "../cli.js"; | ||
|
|
||
| type InferOptionValues<Command extends commander.Command> = ReturnType< | ||
| Command["opts"] | ||
| >; | ||
|
|
||
| type BaseCommand = typeof program; |
There was a problem hiding this comment.
The type BaseCommand references program which is imported from a sibling module. This creates a circular dependency risk since the CLI module also imports from this types module. Consider defining the command type interface directly here instead of importing it.
| import type { program } from "../cli.js"; | |
| type InferOptionValues<Command extends commander.Command> = ReturnType< | |
| Command["opts"] | |
| >; | |
| type BaseCommand = typeof program; | |
| type InferOptionValues<Command extends commander.Command> = ReturnType< | |
| Command["opts"] | |
| >; | |
| type BaseCommand = commander.Command< | |
| [], | |
| Record<string, unknown>, | |
| Record<string, unknown> | |
| >; |
Copilot uses AI. Check for mistakes.
| assert.equal(result.length, 1, "Expected exactly one library file"); | ||
| return await result[0]; |
There was a problem hiding this comment.
The assertion checks result.length but result is an array of promises, not resolved file paths. The check should be performed after awaiting the promises. Move this assertion after return await result[0]; or await all promises first.
| assert.equal(result.length, 1, "Expected exactly one library file"); | |
| return await result[0]; | |
| const resolvedResults = await Promise.all(resultPromises); | |
| assert.equal(resolvedResults.length, 1, "Expected exactly one library file"); | |
| return resolvedResults[0]; |
Copilot uses AI. Check for mistakes.
| const architecture = ANDROID_ARCHITECTURES[target]; | ||
|
|
||
| return [ | ||
| // Use the XCode as generator for Apple platforms |
There was a problem hiding this comment.
The comment mentions 'XCode' and 'Apple platforms' but this is in the Android platform implementation. The comment should be corrected to reference Android/Ninja instead.
| // Use the XCode as generator for Apple platforms | |
| // Use the Ninja generator for Android platforms |
Copilot uses AI. Check for mistakes.
packages/cmake-rn/src/cli.ts
Outdated
| options: baseOptions as BaseOpts & | ||
| ReturnType<typeof platform.buildArgs>, |
There was a problem hiding this comment.
The type assertion baseOptions as BaseOpts & ReturnType<typeof platform.buildArgs> is unsafe because platform.buildArgs returns a string array, not an options object. This suggests a type modeling issue in the platform abstraction.
| options: baseOptions as BaseOpts & | |
| ReturnType<typeof platform.buildArgs>, | |
| options: { | |
| ...baseOptions, | |
| ...platform.buildArgs(), | |
| } as BaseOpts, |
Copilot uses AI. Check for mistakes.
Avoid code-signing when prebuilding
As a preparation for #175 I propose introducing the a "platform" interface, implementable to add support for a subset of targets.